Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: port collect-profiles.sh to 'ipfs diag profile' #8786

Merged
merged 4 commits into from
Apr 12, 2022
Merged

feat: port collect-profiles.sh to 'ipfs diag profile' #8786

merged 4 commits into from
Apr 12, 2022

Conversation

guseggert
Copy link
Contributor

No description provided.

@guseggert guseggert requested a review from lidel March 11, 2022 20:15
@guseggert guseggert added this to the go-ipfs 0.13 milestone Mar 11, 2022
@guseggert guseggert linked an issue Mar 11, 2022 that may be closed by this pull request
Add endpoint for enabling block profiling #8759
Closed
2 tasks
@BigLep
Copy link
Contributor

BigLep commented Mar 17, 2022

2022-03-17 conversation: going to get rid of the shell script. Moved back to in progress.

@guseggert guseggert force-pushed the feat/block-collect-profiles branch 5 times, most recently from 5989330 to e78e2e1 Compare March 18, 2022 16:34
@guseggert guseggert changed the title feat: add block profiling to collect-profiles.sh feat: port collect-profiles.sh to 'ipfs diag profile' Mar 22, 2022
@BigLep BigLep removed the request for review from lidel March 29, 2022 17:00
@lidel lidel self-assigned this Apr 11, 2022
guseggert and others added 3 commits April 11, 2022 23:47
@guseggert @lidel
feat: add block profiling to collect-profiles.sh
55aac0a
@guseggert @lidel
feat: add more profiles to 'ipfs diag profile'
7200592 
This adds mutex and block profiles, and brings the command up-to-par
with 'collect-profiles.sh', so that we can remove it.

Profiles are also now collected concurrently, which improves the
runtime from (profile_time * num_profiles) to just (profile_time).

Note that this has a backwards-incompatible change, removing
--cpu-profile-time in favor of the more general --profile-time, which
covers all sampling profiles.
@lidel
docs(cli): ipfs diag profile
defc2ca
@lidel lidel force-pushed the feat/block-collect-profiles branch from 2dbbbbe to defc2ca Compare April 11, 2022 21:47
lidel
lidel approved these changes Apr 11, 2022
View reviewed changes
Copy link
Member

@lidel lidel left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (tweaked helptext a tiny bit).

Before we merge, it would be useful to get additional review / :+1: from devrel folks (@Jorropo), just to keep them in the loop.
(ipfs diag profile will be the new go-to command that we ask people to run while debugging issues)

@lidel lidel requested a review from Jorropo April 11, 2022 22:03
@Jorropo
Copy link
Contributor

Jorropo commented Apr 11, 2022

@lidel

I'm not sure what does this PR does, I belive it's mainly a refactor / shuffling code arround kind of things.

My issues of ipfs diag profile are:

  1. CPU profiling often blocks for multiple minutes on high load scenario and --profiling-time 0s doesn't disable it.
  2. It doesn't pipeline writing to the output file (as writing to the output file while it is profiling and instead of buffering then writing).
  3. The output file is not packaged like most unix files (with a profile-$(date)/ prefix on all paths) so unzipping it dump everything in pwd (altho that unix tools doing things the wrong way to start with)
  4. The output file is not pipeline friendly.
  5. The output file doesn't compress identical shared data between different files.

Point 1 can be fixed with a if 0 == arround the CPU profiling.
Point 2 to 5 can be fixed by outputing .tar.gz files (pipeling is not just using .tar.gz but would help a lot).

I've actually being using the shell script instead (or just curling the API myself), but I can't recomend that to new people that are having issues because getting them to download the script and execute it, takes any saving I get from using a tar.

@guseggert
Copy link
Contributor Author

guseggert commented Apr 12, 2022 •
edited
Loading

IIUC we didn't use .tar.gz for Windows compatibility. We could add flags for that but doesn't seem worth the effort IMO, it's nice to have a standard format. This new code does stream output instead of buffering though (when possible...stack traces are still buffered internally by the stdlib).

Point 1 is already addressed in this PR, if --profile-time=0 then CPU profiling is not run (and neither is any other sampling profile...but heap profiles are still written).

I think it would be nice though to control exactly what profiles are run, which would be easy to add, so I'll add that.

@guseggert guseggert force-pushed the feat/block-collect-profiles branch 2 times, most recently from 5f418c2 to 163ff1c Compare April 12, 2022 13:46
@guseggert guseggert requested review from lidel and Jorropo and removed request for Jorropo April 12, 2022 14:25
@guseggert guseggert removed the request for review from aschmahmann April 12, 2022 14:26
lidel
lidel approved these changes Apr 12, 2022
View reviewed changes
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Keeping .zip for now is fine (windows interop, allows user to reason what they sent to a third-party)
  • --collectors works as expected, thanks!

Let's merge this, a huge improvement when compared to bin/collect-profiles.sh

@guseggert guseggert merged commit bb68a68 into master Apr 12, 2022
@guseggert guseggert deleted the feat/block-collect-profiles branch April 12, 2022 15:58
xrazis pushed a commit to xrazis/kubo that referenced this pull request Apr 14, 2022
@guseggert @lidel @xrazis
feat: port collect-profiles.sh to 'ipfs diag profile' (ipfs#8786)
b0bb0e2 
* feat: add block profiling to collect-profiles.sh

* feat: add more profiles to 'ipfs diag profile'

This adds mutex and block profiles, and brings the command up-to-par
with 'collect-profiles.sh', so that we can remove it.

Profiles are also now collected concurrently, which improves the
runtime from (profile_time * num_profiles) to just (profile_time).

Note that this has a backwards-incompatible change, removing
--cpu-profile-time in favor of the more general --profile-time, which
covers all sampling profiles.

* docs(cli): ipfs diag profile

* add CLI flag to select specific diag collectors

Co-authored-by: Marcin Rataj <lidel@lidel.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aschmahmann aschmahmann aschmahmann left review comments

@lidel lidel lidel approved these changes

@Jorropo Jorropo Awaiting requested review from Jorropo

Assignees

@lidel lidel

@guseggert guseggert

Labels
None yet
Projects
No open projects
IPFS Shipyard Team
Archived in project
Milestone
go-ipfs 0.13
Development

Successfully merging this pull request may close these issues.

Add endpoint for enabling block profiling
5 participants
@guseggert @BigLep @Jorropo @lidel @aschmahmann